fix: enforce spec compliance in GitHub module#178
Conversation
… comments) - projects.ts: replace console.warn with proper error return on failed Status field mutation, ensuring callers see ok:false instead of silent continuation - discussions.ts: add cursor-based auto-pagination to listDiscussions so all pages are fetched instead of only the first - comments.ts: add runtime validation of comment type field against the known set, rejecting unrecognized types with null instead of blindly casting via `as` - types.ts: extend MaxsimCommentMeta type union with phase-complete and checkpoint variants - Update all corresponding unit tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🎉 This PR is included in version 5.13.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Pull request overview
This PR tightens GitHub-module spec compliance by turning previously “best-effort” behaviors into explicit, handleable results, and by ensuring GitHub Discussions listing returns complete data via pagination.
Changes:
ensureProjectBoard: returnok: falsewhen the GraphQL mutation to add missing Status options fails (instead of logging a warning and continuing).listDiscussions: add cursor-based auto-pagination usingpageInfo { hasNextPage endCursor }to fetch all pages.parseCommentMeta/ types: add runtime validation for commenttypeand extend the allowedMaxsimCommentMeta.typevariants.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/github/projects.ts | Converts a failed Status-option GraphQL update into a structured error result. |
| packages/cli/src/github/discussions.ts | Implements cursor-based pagination for discussions listing. |
| packages/cli/src/github/comments.ts | Adds runtime validation for comment metadata type. |
| packages/cli/src/github/types.ts | Extends MaxsimCommentMeta.type union with phase-complete and checkpoint. |
| packages/cli/tests/unit/github-projects.test.ts | Adds coverage for GraphQL mutation failure path in ensureProjectBoard. |
| packages/cli/tests/unit/github-discussions.test.ts | Adds coverage ensuring listDiscussions paginates and aggregates results. |
| packages/cli/tests/unit/comments.test.ts | Adds coverage for rejecting invalid comment type values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return { | ||
| ok: false, | ||
| error: `Failed to add missing Status options (${missingColumns.map((c) => c.name).join(', ')}): ${updateResult.error}`, | ||
| code: 'UNKNOWN', |
There was a problem hiding this comment.
ensureProjectBoard hard-codes code: 'UNKNOWN' when the GraphQL update fails, which loses the more specific updateResult.code classification from ghExec (e.g., FORBIDDEN/RATE_LIMITED). Propagate updateResult.code instead so callers can handle failures consistently.
| code: 'UNKNOWN', | |
| code: updateResult.code ?? 'UNKNOWN', |
| hasNextPage = pageInfo?.hasNextPage ?? false; | ||
| endCursor = pageInfo?.endCursor ?? null; |
There was a problem hiding this comment.
The pagination loop can become infinite if GitHub returns hasNextPage: true but endCursor is null/empty (then after is never sent and the same page is requested repeatedly). Add a guard that returns an error (or breaks) when hasNextPage is true but endCursor is missing.
| hasNextPage = pageInfo?.hasNextPage ?? false; | |
| endCursor = pageInfo?.endCursor ?? null; | |
| const nextHasNextPage = pageInfo?.hasNextPage ?? false; | |
| const nextEndCursor = pageInfo?.endCursor ?? null; | |
| // Guard against a potential infinite loop if GitHub reports hasNextPage | |
| // but does not provide an endCursor for the next page. | |
| if (nextHasNextPage && !nextEndCursor) { | |
| break; | |
| } | |
| hasNextPage = nextHasNextPage; | |
| endCursor = nextEndCursor; |
| }; | ||
| } | ||
|
|
||
| const VALID_COMMENT_TYPES = new Set(['plan', 'research', 'context', 'progress', 'verification', 'summary', 'error', 'escalation', 'handoff', 'user-intent', 'phase-complete', 'checkpoint'] as const); |
There was a problem hiding this comment.
VALID_COMMENT_TYPES duplicates the MaxsimCommentMeta['type'] union in types.ts, which can drift over time. Consider defining a single exported const array of comment types and deriving both the union type and the runtime Set/type-guard from it to keep them in sync.
Summary
console.warnwith properok: falseerror return when GraphQL mutation to add missing Status field options fails, so callers handle the failure instead of silently continuinglistDiscussionsusingpageInfo { hasNextPage endCursor }, fetching all pages instead of only the firsttypefield inparseCommentMetaagainst a known set of valid types, returningnullfor unrecognized types instead of blindly castingMaxsimCommentMeta.typeunion withphase-completeandcheckpointvariantsTest plan
ensureProjectBoardreturnsok: falsewhen GraphQL mutation failslistDiscussionspaginates through multiple pages and returns all resultsparseCommentMetareturnsnullfor invalid type strings🤖 Generated with Claude Code